Control currently viewed event via RoomViewStore #1058

lukebarnard1 · 2017-06-08T13:30:31Z

Due to the way MatrixChat does a state update when the view_room dispatch fires and a second update when RoomViewStore sends an update, the current event ID and room ID were becoming out of sync. The solution devised was to have the event ID managed by the RoomViewStore itself and do any defaulting there (for when we revisit a room that we saved scroll state for previously).

This required a few changes:

The addition of update_scroll_state in RoomViewStore allows the RoomView to save scroll state for a room before swapping to another one. Previously the caching of scroll state was done in RoomView.
The view_room dispatch now accepts an event_id, which dictates which event is supposed to be scrolled to in the MessagePanel when a new room is viewed. It also accepts event_offset, but currently, this isn't passed in by a dispatch in the app, but it is clobbered when loading the default position when an event_id isn't specified. Finally, highlighted was added to distinguish whether the initial event being scrolled to is also highlighted. This flag is also used by viewRoom in MatrixChat in order to decide whether to notifyNewScreen with the specified event_id.

Fix for element-hq/element-web#4224 Due to the way `MatrixChat` does a state update when the `view_room` dispatch fires and a second update when `RoomViewStore` sends an update, the current event ID and room ID were becoming out of sync. The solution devised was to have the event ID managed by the `RoomViewStore` itself and do any defaulting there (for when we revisit a room that we saved scroll state for previously). This required a few changes: - The addition of `update_scroll_state` in `RoomViewStore` allows the `RoomView` to save scroll state for a room before swapping to another one. Previously the caching of scroll state was done in `RoomView`. - The `view_room` dispatch now accepts an `event_id`, which dictates which event is supposed to be scrolled to in the `MessagePanel` when a new room is viewed. It also accepts `event_offset`, but currently, this isn't passed in by a dispatch in the app, but it is clobbered when loading the default position when an `event_id` isn't specified. Finally, `highlighted` was added to distinguish whether the initial event being scrolled to is also highlighted. This flag is also used by `viewRoom` in `MatrixChat` in order to decide whether to `notifyNewScreen` with the specified `event_id`.

Conflicts: src/components/structures/MatrixChat.js

richvdh · 2017-06-08T13:50:28Z

src/components/structures/MatrixChat.js

@@ -618,40 +618,21 @@ module.exports = React.createClass({
        this.focusComposer = true;

        const newState = {
-            initialEventId: roomInfo.event_id,
-            highlightedEventId: roomInfo.event_id,
-            initialEventPixelOffset: undefined,
            page_type: PageTypes.RoomView,
            thirdPartyInvite: roomInfo.third_party_invite,
            roomOobData: roomInfo.oob_data,
            currentRoomAlias: roomInfo.room_alias,


I think this is redundant now?

richvdh · 2017-06-08T13:55:37Z

src/components/structures/MatrixChat.js

@@ -680,7 +661,7 @@ module.exports = React.createClass({
                }
            }

-            if (roomInfo.event_id) {
+            if (roomInfo.event_id && roomInfo.highlighted) {


can you add this to the jsdoc please

richvdh · 2017-06-08T13:58:46Z

src/components/structures/MatrixChat.js

@@ -1137,6 +1118,7 @@ module.exports = React.createClass({
            const payload = {
                action: 'view_room',
                event_id: eventId,
+                highlighted: Boolean(eventId),


comment to explain what's going on here would be helpful

richvdh · 2017-06-08T14:00:11Z

src/components/structures/RoomView.js

+            isEventHighlighted: RoomViewStore.isEventHighlighted(),
+        };
+
+        if (this.state.eventId !== newState.eventId) {


comment please

richvdh · 2017-06-08T14:04:27Z

src/components/structures/RoomView.js

@@ -598,6 +578,14 @@ module.exports = React.createClass({
        });
    },

+    _updateScrollMap(roomId) {
+        dis.dispatch({


we need some sort of check that roomId isn't null/undef

richvdh · 2017-06-08T14:20:37Z

src/stores/RoomViewStore.js

+            if (!payload.event_id) {
+                const roomScrollState = this._state.scrollStateMap[payload.room_id];
+                if (roomScrollState) {
+                    payload.event_id = roomScrollState.focussedEvent;


please don't write to payload

richvdh · 2017-06-08T14:23:33Z

src/stores/RoomViewStore.js

@@ -27,12 +27,32 @@ const INITIAL_STATE = {
    joinError: null,
    // The room ID of the room
    roomId: null,
+    // The event being viewed
+    eventId: null,


suggest calling this something like initialEventId

richvdh · 2017-06-08T14:23:42Z

src/stores/RoomViewStore.js

+    // The event being viewed
+    eventId: null,
+    // The offset to display the event at (see scrollStateMap)
+    eventPixelOffset: null,


richvdh · 2017-06-08T14:25:06Z

src/stores/RoomViewStore.js

+    eventId: null,
+    // The offset to display the event at (see scrollStateMap)
+    eventPixelOffset: null,
+    // Whether to highlight the event


the initial event

richvdh · 2017-06-08T14:26:13Z

src/stores/RoomViewStore.js

@@ -176,6 +229,18 @@ class RoomViewStore extends Store {
        return this._state.roomId;
    }

+    getEventId() {


comments here might be helpful?

…update from RoomViewStore

Otherwise we pointlessly assign the null key to something

…), alter a copy

richvdh · 2017-06-08T16:02:36Z

src/components/structures/MatrixChat.js

@@ -607,6 +607,7 @@ module.exports = React.createClass({
    // @param {boolean=} roomInfo.show_settings Makes RoomView show the room settings dialog.
    // @param {string=} roomInfo.event_id ID of the event in this room to show: this will cause a switch to the
    //                                    context of that particular event.
+    // @param {boolean=} roomInfo.highlighted If true, add event_id to the hash of the URL


surely it means way more than that? like, the event will also be highlighted in the view.

Bear in mind this is documenting the parameters on the view_room more than the parameters to the function - the docs just got left behind when view_room started going to more places :/

I don't think we should document things that happen elsewhere. If someone wants to know what the view_room dispatch does, they should look for all the things that receive it (namely, RoomViewStore).

I'm not convinced by that. The view_room dispatch is an API, like any other. You don't refuse to document your methods and say "look at these other methods which I call". I want to know: what does it mean, conceptually, to set the 'highlighted' param in a view_room dispatch. I don't think at its heart it means 'add the event_id' to the location bar.

There's a (very strong) argument that this is not the place for that documentation, but we have nowhere else right now.

OK, but I feel that someone is going to get confused when the documentation for this function doesn't match what the function does.

richvdh · 2017-06-08T16:04:13Z

src/components/structures/MatrixChat.js

@@ -1118,6 +1118,7 @@ module.exports = React.createClass({
            const payload = {
                action: 'view_room',
                event_id: eventId,
+                // If an event ID is set (truthy), mark it as highlighted


well, now it has a comment, but I'm not sure I'm any the wiser - the comment says what the code is doing (which I could see for myself), but not why.

So that it is highlighted in the MessagePanel?

I guess I'm after something like

// if there is an event_id in the URL, tell the room view to highlight it

richvdh · 2017-06-08T16:04:50Z

src/components/structures/RoomView.js

@@ -94,6 +94,13 @@ module.exports = React.createClass({
            roomLoading: true,
            peekLoading: false,

+            // The event to be scrolled to initially
+            eventId: null,


should be initial* now I think

richvdh · 2017-06-08T16:07:17Z

src/stores/RoomViewStore.js

-    // Whether to highlight the event
-    isEventHighlighted: false,
+
+    // The event to scroll to initially


could you clarify 'initially' here: I think you mean 'when the room is first viewed' or sth?

richvdh · 2017-06-08T16:09:00Z

src/stores/RoomViewStore.js

+            const newState = {
+                roomId: payload.room_id,
+                initialEventId: payload.event_id,
+                initialEventPixelOffset: payload.event_offset,


is payload.event_offset ever set? I don't think so. Better to use an explicit undefined here.

richvdh · 2017-06-08T16:10:22Z

src/stores/RoomViewStore.js

    getRoomId() {
        return this._state.roomId;
    }

-    getEventId() {
-        return this._state.eventId;
+    // The event to scroll to initially


"initially"

richvdh

lgtm

richvdh · 2017-06-08T17:08:48Z

src/components/structures/MatrixChat.js

-                // If an event ID is set (truthy), mark it as highlighted
+                // If an event ID is given in the URL hash, notify RoomViewStore to mark
+                // it as highlighted, which will propagate to RoomView and highlight the
+                // associated EventTile.


perfect ⭐

lukebarnard1 assigned dbkr and richvdh Jun 8, 2017

Luke Barnard added 2 commits June 8, 2017 14:30

Remove redundant action id

f320f7d

Merge branch 'develop' into luke/fix-event-id-state

ea97d16

Conflicts: src/components/structures/MatrixChat.js

ara4n changed the title ~~Control currently viewied event via RoomViewStore~~ Control currently viewed event via RoomViewStore Jun 8, 2017

richvdh suggested changes Jun 8, 2017

View reviewed changes

Luke Barnard added 10 commits June 8, 2017 15:30

Remove redundant currentRoomAlias

aa20ad7

Doc _viewRoom highlighted

8ddc38c

Explain setting of highlighted in room/$eventId showScreen handling.

2e82bf4

Add RoomView initial state and docs

dca2be6

Explain why search results are cleared when eventId changes

e58e4db

Add comment about roomId potentially not being resolved following an …

59e649f

…update from RoomViewStore

Null-guard roomId before sending a dispatch to update scroll state

1792fa4

Otherwise we pointlessly assign the null key to something

Rename RVS state (event -> initialEvent) and redocument

000a045

Don't alter members of payload (when defaulting an event to scroll to…

36f72cc

…), alter a copy

Update docs for RVS

1ff2be1

lukebarnard1 assigned richvdh and unassigned dbkr and richvdh Jun 8, 2017

lukebarnard1 mentioned this pull request Jun 8, 2017

Better error messages for login #1060

Merged

richvdh reviewed Jun 8, 2017

View reviewed changes

Luke Barnard added 6 commits June 8, 2017 17:26

Rename initial state variables

c02dbd1

Clarify initial event

8cfdb4c

Explicit undefined initialEventPixelOffset

cd8093d

Update doc for getInitialEventId

c72edab

Highlighted means highlighted

9892566

Clarify doc for highlighted

5588e11

richvdh approved these changes Jun 8, 2017

View reviewed changes

lukebarnard1 merged commit ce09773 into develop Jun 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control currently viewed event via RoomViewStore #1058

Control currently viewed event via RoomViewStore #1058

lukebarnard1 commented Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

lukebarnard1 Jun 8, 2017 •

edited

Loading

richvdh Jun 8, 2017

lukebarnard1 Jun 8, 2017

richvdh Jun 8, 2017

lukebarnard1 Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh Jun 8, 2017

richvdh left a comment

richvdh Jun 8, 2017

Control currently viewed event via RoomViewStore #1058

Control currently viewed event via RoomViewStore #1058

Conversation

lukebarnard1 commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Jun 8, 2017 •

edited

Loading